Fix acoustic source and body force timing#1232
Fix acoustic source and body force timing#1232sbryngelson wants to merge 9 commits intoMFlowCode:masterfrom
Conversation
codeant-ai
bot
added
the
size:XS
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Updates simulation time handling so source terms/body forces see the correct physical time during TVD RK sub-steps, especially under adaptive time stepping.
Changes:
- Advance
mytimeafter the TVD RK call rather than before it. - Compute acoustic source
sim_timefrommytimeinstead oft_step*dt.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/simulation/m_start_up.fpp | Delays mytime advancement until after RK stepping so sub-steps use the correct time. |
| src/simulation/m_acoustic_src.fpp | Switches acoustic source time calculation to use the evolving simulation clock (mytime) for adaptive dt. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/simulation/m_acoustic_src.fpp (1)
131-145:⚠️ Potential issue | 🟡 MinorRemove unused
t_stepdummy argument froms_acoustic_src_calculationsThe
t_stepparameter at line 145 is no longer used in the subroutine body after the change tosim_time = mytimeat line 170. This dead code will trigger "unused dummy argument" compiler warnings with strict Fortran flags.Changes required
In
src/simulation/m_acoustic_src.fpp(lines 131, 145):- impure subroutine s_acoustic_src_calculations(q_cons_vf, q_prim_vf, t_step, rhs_vf) + impure subroutine s_acoustic_src_calculations(q_cons_vf, q_prim_vf, rhs_vf) - integer, intent(in) :: t_stepIn
src/simulation/m_rhs.fpp(lines 1026–1029), update the call site:call s_acoustic_src_calculations(q_cons_qp%vf(1:sys_size), & q_prim_qp%vf(1:sys_size), & - t_step, & rhs_vf)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/simulation/m_acoustic_src.fpp` around lines 131 - 145, The subroutine s_acoustic_src_calculations still declares the dummy argument t_step but no longer uses it; remove t_step from the subroutine signature and its intent declaration in s_acoustic_src_calculations and delete any references to t_step inside the routine, then update every call site to that routine (e.g., the call in m_rhs where s_acoustic_src_calculations is invoked) by removing the corresponding t_step actual argument; also adjust any module/procedure interfaces or declarations that list s_acoustic_src_calculations so the new 3-argument signature (q_cons_vf, q_prim_vf, rhs_vf) is consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/simulation/m_acoustic_src.fpp`:
- Around line 131-145: The subroutine s_acoustic_src_calculations still declares
the dummy argument t_step but no longer uses it; remove t_step from the
subroutine signature and its intent declaration in s_acoustic_src_calculations
and delete any references to t_step inside the routine, then update every call
site to that routine (e.g., the call in m_rhs where s_acoustic_src_calculations
is invoked) by removing the corresponding t_step actual argument; also adjust
any module/procedure interfaces or declarations that list
s_acoustic_src_calculations so the new 3-argument signature (q_cons_vf,
q_prim_vf, rhs_vf) is consistent.
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (3)
tests/128954AD/golden-metadata.txt (1)
176-192: HPC node running with several unmitigated CPU speculative-execution vulnerabilities.The
lscpuoutput records that Spectre v1/v2, Retbleed, Spec Store Bypass, Gather Data Sampling, and Vmscape are all "Vulnerable" without kernel mitigations on this compute node. While this is captured metadata (not code), shared HPC nodes with these vulnerabilities and no IBPB/STIBP mitigations can leak data between co-located jobs. Worth raising with your HPC administrators.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/128954AD/golden-metadata.txt` around lines 176 - 192, The golden metadata shows multiple unmitigated CPU speculative-execution issues (entries like "Vulnerability Spectre v1", "Vulnerability Spectre v2", "Vulnerability Retbleed", "Vulnerability Spec store bypass", "Vulnerability Gather data sampling", "Vulnerability Vmscape"); update the project artefacts to surface this operational risk: add a short conspicuous note in the repository (e.g., README or a CONTRIBUTING/INFRA.md) and prepend the tests/128954AD/golden-metadata.txt file with a one-line warning that this HPC node is running with known speculative-execution vulnerabilities and that cluster administrators must be contacted to enable IBPB/STIBP/other mitigations, and include suggested mitigations (IBPB/STIBP, microcode/kernel updates) so operators know next steps.tests/07C54EDD/golden-metadata.txt (1)
176-192: Informational: several CPU vulnerabilities lack mitigations on the test host.The
lscpuoutput reports multiple Spectre/Retbleed/MMIO-class vulnerabilities asVulnerablewithout active mitigations (e.g., IBPB/STIBP disabled for Spectre v2, no swapgs barriers for Spectre v1). This is a PACE cluster environment concern and is unrelated to the code changes in this PR. No action needed here, but the platform team may want to assess whether kernel/microcode mitigations should be enabled for the test infrastructure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/07C54EDD/golden-metadata.txt` around lines 176 - 192, The test host's lscpu/golden-metadata.txt shows several CPU vulnerabilities reported as "Vulnerable" (Spectre v1/v2, Retbleed, MMIO sampling) with mitigations disabled; this is an environment/platform issue not caused by this PR—do not change code or tests; instead annotate the PR/test metadata by adding a short note in golden-metadata.txt (or the PR description) that these findings are informational and related to the test host kernel/microcode so reviewers and the platform team are aware.tests/CE7B0BC7/golden-metadata.txt (1)
9-9: Non-standard section ordering in the metadata file.Sections are ordered
pre_process → post_process → syscheck → simulation(lines 9, 43, 77, 111), whereas the natural pipeline execution order ispre_process → simulation → post_process → syscheck. The AI summary confirms this reordering is consistent across all affected golden-metadata files in the PR, so it appears to be an artifact of the test framework's output rather than a manual edit. If the ordering is driven by the framework, this is fine; otherwise, aligning the section order with the pipeline execution sequence would improve readability.Also applies to: 43-43, 77-77, 111-111
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/CE7B0BC7/golden-metadata.txt` at line 9, The metadata sections are in non-execution order (currently pre_process → post_process → syscheck → simulation); update the golden metadata so sections appear in pipeline execution order: pre_process → simulation → post_process → syscheck by reordering the section headers "pre_process", "simulation", "post_process", and "syscheck" in the golden-metadata files (or, if the test framework generates this order, fix the generator to emit them in that execution sequence).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/0501B3DA/golden.txt`:
- Line 6: The golden-file comparison is brittle because cons.2.00.000001.dat
(line 6) and prim.2.00.000001.dat (line 26) contain signed -0.0 entries; fix by
normalizing signed zeros or using tolerance-based numeric comparison: update the
test-comparison routine (or golden-file writer) to convert any floating value
equal to 0.0 (e.g., value == 0.0 || Math.abs(value) < DBL_EPSILON) to the
canonical "0.0" string, or change the comparator to compare parsed floats with
absolute-difference ≤ ε instead of doing raw string equality, so differing
signs-of-zero cannot cause spurious failures.
In `@tests/07C54EDD/golden-metadata.txt`:
- Line 7: The golden-metadata.txt shows the git ref
"c9cff0655061b66969305f06a0db63138eeda7e4 on fix/time-stepping-order (dirty)",
meaning the golden outputs were generated with uncommitted changes; either (A)
revert or commit local modifications and regenerate the golden files from a
clean commit so the CI will reproduce them exactly, or (B) audit the uncommitted
changes to confirm they only affect non-simulation artifacts (e.g., build files)
and then update the metadata to a clean ref; locate the golden-metadata.txt
entry with that git ref and re-run the golden-file generation step from a clean
working tree before merging.
In `@tests/128954AD/golden-metadata.txt`:
- Line 7: The golden metadata shows the Git commit was recorded with a dirty
working tree; to fix this, clean or commit/squash any local changes, re-run the
golden file generation so tests/128954AD/golden-metadata.txt is recreated
without the "(dirty)" suffix, and commit the regenerated golden files; ensure
the generate step runs from a clean checkout (or a committed branch) so the new
metadata records a clean commit hash.
In `@tests/53A15FFC/golden-metadata.txt`:
- Line 7: The golden metadata indicates the golden files were generated from a
dirty working tree ("Git: ... (dirty)"), which can make test baselines
non-reproducible; to fix, regenerate the golden outputs from a clean checkout
(checkout or stash/commit all local changes), re-run the golden file generation,
and update tests/53A15FFC/golden-metadata.txt (and any updated golden files) so
the Git line reflects a clean commit SHA for branch fix/time-stepping-order and
include those regenerated files in the commit before pushing.
In `@tests/A078904B/golden-metadata.txt`:
- Line 7: The golden metadata file records the Git status as "(dirty)" which
indicates uncommitted changes; to fix, checkout a clean working tree (commit or
stash all local changes), regenerate the golden file so the Git line in
tests/A078904B/golden-metadata.txt no longer includes "(dirty)" (or update the
Git entry to match the committed HEAD), and commit the regenerated golden file
so the metadata reproducibly corresponds to a specific code state; specifically
ensure the line containing "(dirty)" (the Git status string) is produced from a
clean repo before committing the updated golden file.
In `@tests/B2EC143C/golden-metadata.txt`:
- Line 29: The golden metadata contains a personal HPC scratch path for "Fypp"
(/storage/home/hcoda1/6/sbryngelson3/scratch/MFC-prs/build/venv/bin/fypp);
update the golden-metadata generation (or the file
tests/B2EC143C/golden-metadata.txt) so the Fypp entry is either normalized to a
CI/system path, made relative, or replaced with a placeholder (e.g., "Fypp :
<fypp-path>" or a CI-known path) to avoid embedding user-specific scratch
directories; locate the "Fypp" lines (currently repeated at the four build
sections) and ensure the generation code emits a canonical/placeholder path
instead of the personal scratch path.
- Around line 13-16: The golden file regeneration merged timing-fix effects with
a broad environment change (ISA and toolchain differences) across 561 tests;
update tests/B2EC143C/golden-metadata.txt (and the README or CI test
documentation) to explicitly state that the golden diffs include both the timing
fix and the platform/compiler change (AppleClang v17.0.0 -> GNU v12.3.0, CMake
v3.31.3 -> v3.26.5, and CPU ISA change), or alternatively re-run and regenerate
the golden outputs on a single consistent CI platform to isolate the timing-fix
impact—pick one approach and document it clearly so reviewers know the delta’s
provenance.
In `@tests/B9553426/golden-metadata.txt`:
- Line 7: The golden-metadata.txt shows the Git commit line with a "(dirty)"
annotation which indicates the golden files were generated from an uncommitted
working tree; regenerate the golden outputs from a clean, committed state and
update tests/B9553426/golden-metadata.txt to record the clean commit SHA without
"(dirty)"; additionally add or update the golden-file generation workflow (or
test helper) to fail when the working tree is dirty (detect "(git status
--porcelain)" non-empty) so future golden files cannot be produced from a dirty
tree.
In `@tests/C7927D03/golden-metadata.txt`:
- Line 7: The golden metadata shows a dirty working tree ("Git:
c9cff0655061b66969305f06a0db63138eeda7e4 on fix/time-stepping-order (dirty)");
commit or stash the pending changes, check out the intended commit
(c9cff0655061b66969305f06a0db63138eeda7e4) or recreate the baseline from a clean
tree, then re-run the golden file generation so
tests/C7927D03/golden-metadata.txt no longer contains "(dirty)" and accurately
records the clean commit hash.
In `@tests/CE7B0BC7/golden-metadata.txt`:
- Line 7: The golden file metadata shows a "(dirty)" marker for commit
c9cff0655061b66969305f06a0db63138eeda7e4 which makes golden outputs
unreproducible; update the golden-generation/test harness that writes
tests/CE7B0BC7/golden-metadata.txt (or the routine that calls git status) to
detect a dirty working tree and fail or emit a CI-visible error instead of
accepting the dirty state—either abort generation when git reports modified
files, automatically stash/commit changes before writing the golden metadata, or
add a CI check that validates no "(dirty)" is present in generated
golden-metadata.txt and fails the pipeline if it is.
- Line 29: The golden metadata contains an absolute, user-specific Fypp path
("Fypp : /storage/home/.../fypp") which breaks portable exact-match tests;
update the metadata-generation or test-comparison logic to either (a)
canonicalize the Fypp entry before writing/ comparing—e.g., replace the absolute
prefix with an environment placeholder like $HOME or $VENV, or make it a
relative path (venv/bin/fypp), or (b) exclude tool-path metadata lines (the
"Fypp" key and other similar tool-path entries) from the golden-file comparison;
locate the code that emits or compares "Fypp" in golden-metadata.txt and
implement one of these normalizations or exclusions so tests no longer depend on
a per-user absolute path.
---
Nitpick comments:
In `@tests/07C54EDD/golden-metadata.txt`:
- Around line 176-192: The test host's lscpu/golden-metadata.txt shows several
CPU vulnerabilities reported as "Vulnerable" (Spectre v1/v2, Retbleed, MMIO
sampling) with mitigations disabled; this is an environment/platform issue not
caused by this PR—do not change code or tests; instead annotate the PR/test
metadata by adding a short note in golden-metadata.txt (or the PR description)
that these findings are informational and related to the test host
kernel/microcode so reviewers and the platform team are aware.
In `@tests/128954AD/golden-metadata.txt`:
- Around line 176-192: The golden metadata shows multiple unmitigated CPU
speculative-execution issues (entries like "Vulnerability Spectre v1",
"Vulnerability Spectre v2", "Vulnerability Retbleed", "Vulnerability Spec store
bypass", "Vulnerability Gather data sampling", "Vulnerability Vmscape"); update
the project artefacts to surface this operational risk: add a short conspicuous
note in the repository (e.g., README or a CONTRIBUTING/INFRA.md) and prepend the
tests/128954AD/golden-metadata.txt file with a one-line warning that this HPC
node is running with known speculative-execution vulnerabilities and that
cluster administrators must be contacted to enable IBPB/STIBP/other mitigations,
and include suggested mitigations (IBPB/STIBP, microcode/kernel updates) so
operators know next steps.
In `@tests/CE7B0BC7/golden-metadata.txt`:
- Line 9: The metadata sections are in non-execution order (currently
pre_process → post_process → syscheck → simulation); update the golden metadata
so sections appear in pipeline execution order: pre_process → simulation →
post_process → syscheck by reordering the section headers "pre_process",
"simulation", "post_process", and "syscheck" in the golden-metadata files (or,
if the test framework generates this order, fix the generator to emit them in
that execution sequence).
sbryngelson
force-pushed
the
fix/time-stepping-order
branch
from
df70e0e to
613b724
Compare
codeant-ai
bot
added
size:XXL
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
tests/B9553426/golden-metadata.txt (1)
176-192: CI host has several unmitigated CPU vulnerabilities — consider hardening or documenting.The
lscpudump records a number of hardware vulnerabilities without active OS/kernel mitigations on the build/test machine:
- Spectre v2:
IBPB: disabled; STIBP: disabled; PBRSB-eIBRS: Vulnerable; BHI: Vulnerable- Retbleed:
Vulnerable- Gather data sampling:
Vulnerable- Mmio stale data:
Vulnerable- Spec store bypass:
Vulnerable- Indirect target selection:
Vulnerable- Vmscape:
VulnerableThese do not affect test correctness, but on a shared HPC cluster running multi-tenant workloads the disabled mitigations (IBPB/STIBP off) increase cross-process information-leakage risk. Consider opening an infrastructure ticket with the PACE/GT cluster admins to review the kernel boot parameters (
spectre_v2=on ibpb=on stibp=on spec_store_bypass_disable=on) on this node, or at minimum document that this is an accepted risk for the CI environment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/B9553426/golden-metadata.txt` around lines 176 - 192, The CI host lscpu output shows multiple unmitigated CPU vulnerabilities (notably Spectre v2 with IBPB/STIBP disabled, Retbleed, Gather data sampling, Mmio stale data, Spec store bypass, Indirect target selection, Vmscape); either harden the CI node by asking infra to enable kernel mitigations via kernel boot parameters (e.g., spectre_v2=on ibpb=on stibp=on spec_store_bypass_disable=on) or explicitly document the accepted risk for the CI environment in the project/infrastructure docs and open an infrastructure ticket referencing this golden-metadata output (call out “Spectre v2: IBPB/STIBP”, “Retbleed”, “Gather data sampling”, “Mmio stale data”, “Spec store bypass”, “Indirect target selection”, “Vmscape”) so the cluster admins can evaluate and, if chosen, apply the suggested boot params.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/37FA2CEF/golden-metadata.txt`:
- Line 163: The golden-metadata.txt contains a dynamic field "CPU(s) scaling
MHz" that varies between runs and will break exact-match tests; update the
metadata comparison logic (the golden metadata comparison routine that
reads/validates golden-metadata.txt) to either strip/ignore lines matching the
dynamic pattern (e.g. any line starting with "CPU(s) scaling MHz:" and other
volatile entries like the timestamp on line 1) before comparing, or mark
golden-metadata.txt as informational only by bypassing exact-match checks for
that file; implement the change in the test harness where golden-metadata.txt is
loaded/validated so comparisons normalize or skip those dynamic fields.
In `@tests/6B1AD553/golden-metadata.txt`:
- Line 7: The golden-metadata file records a Git hash with a "(dirty)" marker,
which indicates the baseline was produced from an uncommitted working tree;
regenerate the golden files from a clean commit and update the golden-metadata
entries (e.g., the line currently showing
"c9cff0655061b66969305f06a0db63138eeda7e4 on fix/time-stepping-order (dirty)" in
tests/6B1AD553/golden-metadata.txt) so the "(dirty)" suffix is removed and the
recorded commit hash corresponds to a clean, committed state; repeat the same
regeneration/update for the other golden-metadata files referenced (016C1B8B,
10041BB7, 12ECE133, AE02324F, 121D4ECA) to ensure CI can reproduce baselines
deterministically.
In `@tests/986BC1A2/golden-metadata.txt`:
- Line 7: The golden-metadata entry contains the literal marker "(dirty)" (e.g.
the line with commit hash c9cff0655061b66969305f06a0db63138eeda7e4 showing
"(dirty)"), which means the file was generated with uncommitted changes;
regenerate the golden-metadata without local modifications by checking out a
clean working tree, committing or stashing any changes, rerunning the --generate
step that produces golden-metadata.txt, and committing the regenerated file so
the "(dirty)" marker is removed and the recorded commit hash is reproducible.
In `@tests/AE02324F/golden-metadata.txt`:
- Line 7: The golden-metadata.txt shows AE02324F (and 121D4ECA) were generated
from commit cad93358 while the other tests used commit c9cff065; verify
consistency by regenerating the AE02324F and 121D4ECA golden outputs from the
same commit used by the other tests (c9cff065) or confirm they are intentionally
from cad93358, then update golden-metadata.txt and the two golden files
accordingly so all tests reflect the same code state; ensure you reference and
rebuild the test artifacts for AE02324F and 121D4ECA and commit the updated
golden outputs and metadata.
---
Duplicate comments:
In `@tests/016C1B8B/golden-metadata.txt`:
- Line 7: The golden-metadata shows the repo state as "dirty" (Git: c9cff065...
on fix/time-stepping-order (dirty)), which duplicates the same
dirty-working-tree concern flagged for tests/6B1AD553; clean up the working tree
for the failing tests by either committing or stashing local changes and
ensuring the golden file updates are intentionally committed for tests/016C1B8B
(and similarly for tests/6B1AD553), then re-run the test to produce a clean,
committed golden-metadata before pushing the branch.
In `@tests/0501B3DA/golden-metadata.txt`:
- Line 29: The Fypp entry in golden-metadata.txt contains a personal HPC scratch
path; update the "Fypp" field to remove the user-specific absolute path and
replace it with a portable value (e.g., just "fypp", an environment-variable
placeholder like "${FYPP_BIN}", or a CI/workspace-agnostic path) so the metadata
contains no personal paths and works across environments; edit the "Fypp" line
to the chosen portable value.
- Line 163: The golden file contains a dynamic field "CPU(s) scaling MHz:" whose
value (88% here) changes per run; update the test/golden comparison to treat the
"CPU(s) scaling MHz:" line as dynamic by either replacing its value with a
stable placeholder (e.g., "CPU(s) scaling MHz: <DYNAMIC>") in the
golden-metadata or updating the comparison logic to ignore/regex-match that line
(match "CPU(s) scaling MHz:\s*.*" or "\d+%") so the test no longer fails on
runtime fluctuations.
- Line 7: The golden metadata contains a dirty working-tree marker "(dirty)" in
the Git line ("Git: c9cff0655061b66969305f06a0db63138eeda7e4 on
fix/time-stepping-order (dirty)"); remove the "(dirty)" marker or update the
metadata generation so it strips the dirty flag (e.g., change how the Git
hash/branch string is produced) so the Git line only contains the commit id and
branch without "(dirty)". Ensure the produced line matches the expected stable
format "Git: <commit> on <branch>" used by the tests.
In `@tests/0501B3DA/golden.txt`:
- Line 6: The golden file contains signed -0.0 entries (e.g., the momentum line
starting with "D/cons.2.00.000001.dat" includes "-0.0"); update the golden
generation/serialization path that writes momentum values (the routine that
emits entries into golden.txt, e.g., the momentum formatting/serialize function)
to normalize negative zero to positive zero before formatting—detect -0.0
(Object.is(value, -0) or value === 0 && 1/value === -Infinity) and output "0.0"
instead of "-0.0", then regenerate the golden.txt so all "-0.0" tokens are
replaced with "0.0".
In `@tests/07C54EDD/golden-metadata.txt`:
- Line 7: The branch contains a dirty working tree (the metadata line
referencing commit c9cff0655061b66969305f06a0db63138eeda7e4 shows uncommitted
changes); clean this by either committing the intended changes with a clear
message, stashing or discarding unintended edits, and re-running the tests so
the golden-metadata.txt no longer reports a dirty state; ensure the duplicate
"dirty working tree" flag is removed before pushing so CI and reviewers see a
clean commit on the fix/time-stepping-order branch.
In `@tests/0D1FA5C5/golden-metadata.txt`:
- Line 7: The golden metadata contains a dirty working-tree marker (" (dirty)")
in the Git line; update the metadata by removing the " (dirty)" suffix or
regenerate the golden-metadata.txt so the Git entry is a clean commit hash
(e.g., "c9cff0655061b66969305f06a0db63138eeda7e4 on fix/time-stepping-order") to
prevent the dirty working-tree marker from being checked in.
- Line 29: Replace the hard-coded personal HPC scratch path in the Fypp field of
golden-metadata.txt with a non-personal, portable value; locate the "Fypp :
/storage/home/..." entry and change it to a generic reference such as "Fypp :
fypp" or an environment-based placeholder like "Fypp : ${FYPP:-fypp}" so the
metadata does not contain a user-specific filesystem path.
- Line 163: The line with the dynamic field "CPU(s) scaling MHz" is
environment-dependent and should be normalized for tests: update the golden
comparison to either replace the numeric percent for the "CPU(s) scaling MHz"
field with a stable placeholder/wildcard (e.g., "<DYNAMIC_PERCENT>") or adjust
the test comparator to strip/ignore the trailing "%"/digits when comparing that
key so the test no longer fails on variable CPU scaling values.
In `@tests/10041BB7/golden-metadata.txt`:
- Line 7: The tests golden-metadata shows a dirty working tree for commit
c9cff0655061b66969305f06a0db63138eeda7e4 on branch fix/time-stepping-order;
clean the working tree by either committing the local changes, stashing them, or
resetting the dirty files so that the repository is clean before updating or
re-running tests, and ensure the same fix/time-stepping-order branch and commit
hash are used consistently (or update the golden-metadata entry to the new clean
commit hash).
In `@tests/121D4ECA/golden-metadata.txt`:
- Line 7: The golden-metadata.txt shows a "dirty" working tree and an
inconsistent commit hash; please ensure your repo is clean and the golden
metadata reflects a committed, reproducible state: stash or commit any local
changes, check out the branch (fix/time-stepping-order) so the commit id
matches, re-run the test/golden generation that writes
tests/121D4ECA/golden-metadata.txt and commit the regenerated file (or update
the hash entry to the exact committed SHA without "dirty"); apply the same fix
pattern used for tests/AE02324F and tests/6B1AD553 so all golden-metadata files
contain exact committed SHAs and no "dirty" markers.
In `@tests/128954AD/golden-metadata.txt`:
- Line 7: The golden metadata contains a non-deterministic " (dirty)"
working-tree marker in the Git line which breaks repeatable tests; update the
test data generation or the assertion to strip working-tree status so the Git
line is deterministic (e.g., remove the " (dirty)" suffix or record only the
commit hash c9cff0655061b66969305f06a0db63138eeda7e4), and adjust the code that
writes/validates golden-metadata.txt so it never records transient dirty state
(locate the code that emits the Git line in the golden metadata and normalize it
to the clean commit hash or a fixed placeholder).
In `@tests/12ECE133/golden-metadata.txt`:
- Line 7: The golden metadata contains a git line with a "(dirty)" suffix ("Git:
c9cff065... on fix/time-stepping-order (dirty)"), which makes the test
non-deterministic; remove the dirty state by committing or stashing any local
changes and regenerate/update the golden metadata so the Git line records the
clean commit hash/branch only, or modify the test to expect a stable placeholder
instead of a dirty marker; look for the golden-metadata handling that writes the
"Git:" line (tests/12ECE133/golden-metadata.txt) and ensure it records only the
commit hash/branch without "(dirty)".
In `@tests/2122A4F6/golden-metadata.txt`:
- Line 29: The Fypp entry currently contains a personal HPC scratch path;
replace the hard-coded path in the Fypp field with an environment-agnostic value
(e.g., just "fypp", an absolute system path like "/usr/bin/fypp", or a
configurable placeholder such as "${FYPP_PATH}") so the metadata no longer
references a user-specific scratch directory and is portable across
environments.
- Line 163: The failing golden metadata contains a dynamic value for the line
starting with "CPU(s) scaling MHz:" which varies per run; update the test/golden
file handling for this field so it no longer asserts the literal "91%" — either
replace the concrete value with a stable placeholder/wildcard (e.g. "CPU(s)
scaling MHz: <DYNAMIC>" or a regex like "CPU(s) scaling
MHz:\\s+.*") or adjust the comparison code to normalize/ignore the "CPU(s)
scaling MHz" line during diffing so the test is stable across runs.
- Line 7: The golden-metadata contains a dirty working-tree marker (" (dirty)")
appended to the Git revision string; remove the " (dirty)" suffix from the Git
line (the string containing the commit hash and branch, e.g. the line with "Git:
c9cff0655061b66969305f06a0db63138eeda7e4 on fix/time-stepping-order (dirty)") so
the file records a clean commit hash and branch only.
In `@tests/37FA2CEF/golden-metadata.txt`:
- Line 29: The Fypp entry in golden-metadata.txt contains a personal HPC scratch
path; replace that hard-coded path (the "Fypp :" field) with a non-personal,
portable value—e.g. the binary name "fypp", an absolute system path, or an
environment/CI variable placeholder (like ${FYPP_PATH})—so the metadata does not
embed user-specific directories and remains reproducible across environments.
- Line 7: The golden metadata contains a dirty working-tree marker ("(dirty)")
after the Git entry for commit c9cff065... on branch fix/time-stepping-order;
remove this by cleaning or committing local changes and then update the Git line
in golden-metadata.txt to a clean commit reference (either remove the " (dirty)"
suffix or replace the hash/branch with the new clean commit hash) so the test no
longer detects a dirty working tree.
In `@tests/4A1BD9B8/golden-metadata.txt`:
- Line 7: The golden-metadata.txt contains a "(dirty)" marker indicating
baselines were generated from an uncommitted working tree; regenerate the golden
files from a clean commit or remove the "(dirty)" suffix so baselines are
reproducible—specifically update the process that writes golden-metadata.txt (or
the test generator that emits the "Git: ... on fix/time-stepping-order (dirty)"
line) to ensure it records a clean commit hash (no "(dirty)") or fail the
generation when the repo is dirty, then regenerate the golden files such that
golden-metadata.txt no longer contains "(dirty)".
In `@tests/53A15FFC/golden-metadata.txt`:
- Line 7: The golden metadata file contains a Git commit line marked with
"(dirty)" in golden-metadata.txt which indicates it was generated from a dirty
working tree; regenerate the golden file from a clean working tree (ensure git
status is clean or stash/commit local changes) and update golden-metadata.txt to
remove the "(dirty)" suffix (or re-run the golden generation step so the commit
line matches a clean-state commit hash), then commit the updated
golden-metadata.txt.
In `@tests/70EC99CE/golden-metadata.txt`:
- Line 7: The golden metadata contains a non-reproducible "Git: ... (dirty)"
entry; update the code that generates or the stored metadata to omit workspace
state or normalize it to a fixed commit hash only—remove the "(dirty)" suffix or
replace the entire Git line with just the commit SHA so tests are reproducible;
look for the metadata writer that emits the "Git:" key (the golden-metadata
generation path that produces the line "Git:
c9cff0655061b66969305f06a0db63138eeda7e4 on fix/time-stepping-order (dirty)")
and change it to strip working-tree status or fail the generation when the tree
is dirty.
In `@tests/A078904B/golden-metadata.txt`:
- Line 7: The golden-metadata.txt Git line currently records a dirty working
tree ("Git: c9cff0... on fix/time-stepping-order (dirty)"); clean the working
tree (commit or stash local changes) on branch fix/time-stepping-order and then
regenerate or update the Git line in tests/A078904B/golden-metadata.txt so it
records the exact commit hash without the "(dirty)" suffix (or update the
metadata generation step to fail if the tree is dirty) to remove the previously
flagged dirty-tree concern.
In `@tests/AE02324F/golden-metadata.txt`:
- Line 7: The golden-metadata.txt contains a Git status line showing a dirty
working tree ("cad933585... on fix/time-stepping-order (dirty)"), which causes
non-deterministic test metadata; update tests/AE02324F/golden-metadata.txt to
reflect a clean commit hash/branch or remove the "(dirty)" marker by ensuring
the repository state is clean before regenerating the golden file (commit,
stash, or reset local changes) and then re-run the generator so the Git line in
golden-metadata.txt matches a clean commit SHA and branch name.
In `@tests/B2EC143C/golden-metadata.txt`:
- Line 29: The Fypp entry in golden-metadata.txt contains a personal HPC scratch
path; update the Fypp field to a portable value (e.g., just "fypp", a relative
venv path, or an environment-placeholder like "${FYPP_PATH}") so the metadata
contains no user-specific absolute paths; change the line that begins with "Fypp
:" to the chosen generic value.
- Line 7: The golden-metadata.txt contains a dirty working-tree marker
("(dirty)") in the Git metadata line; remove the "(dirty)" marker or regenerate
golden-metadata.txt from a clean working tree so the line reads like "Git:
c9cff0655061b66969305f06a0db63138eeda7e4 on fix/time-stepping-order" without
"(dirty)"; if this file is produced by a script, update that generator to run
only on a clean repo (or strip the dirty suffix) so future runs don't commit the
dirty marker.
- Around line 13-16: The golden output is polluted by environment-specific lines
(e.g., "CMake v3.26.5" and the "C : GNU v12.3.0 (...)" / "Fortran : GNU
v12.3.0 (...)" entries in golden-metadata.txt), so update the test to separate
environment changes from the timing-fix verification by either normalizing or
stripping those metadata lines before comparing golden outputs; specifically
modify the test harness or comparison function that reads golden-metadata.txt to
remove or canonicalize the CMake/version and compiler path lines (the "CMake
..." and "C :" / "Fortran :" entries) so the timing-fix-only assertions
compare deterministic content independent of environment, or else add a
dedicated golden file for the timing-fix case that excludes environment
metadata.
- Line 163: The golden file contains a dynamic line "CPU(s) scaling MHz:
100%" which will vary between runs; update the test comparison to normalize or
ignore that field by matching the literal prefix "CPU(s) scaling MHz:" and
replacing the rest with a stable placeholder (or strip percentage/numeric value)
before asserting equality, or remove that line from the golden output; adjust
the test comparator that reads golden-metadata.txt to perform this normalization
(use a regex anchored on "CPU(s) scaling MHz:" to locate the line).
In `@tests/B54BB9D8/golden-metadata.txt`:
- Line 7: The "Git:" line in the generated metadata for test B54BB9D8 contains a
volatile "(dirty)" marker (same issue flagged for 986BC1A2); update the
metadata-generation step that writes the "Git:" line so it either runs in a
clean repo or normalizes the output by stripping the "(dirty)" suffix (e.g.,
modify the function that produces the golden-metadata entry or the generator
that emits the Git SHA to remove any " dirty" tag), ensuring deterministic
metadata for B54BB9D8 and other tests.
In `@tests/B9553426/golden-metadata.txt`:
- Line 7: The golden metadata file tests/B9553426/golden-metadata.txt was
produced from a dirty working tree (it contains the "(dirty)" tag for Git ref
c9cff0655061b66969305f06a0db63138eeda7e4); to fix, clean or stash all
uncommitted changes, checkout the intended commit (or commit your changes),
regenerate the golden files using the same golden-generation script/command you
used originally, and update tests/B9553426/golden-metadata.txt so the Git line
references the clean commit hash without "(dirty)" (or the new commit hash) to
ensure reproducible golden metadata.
In `@tests/C7927D03/golden-metadata.txt`:
- Line 7: The golden-metadata entry contains a duplicate "dirty" working-tree
flag (the line starting with "Git:
c9cff0655061b66969305f06a0db63138eeda7e4 on fix/time-stepping-order (dirty)");
clean up by making your working tree clean (commit or stash local changes) and
then update or regenerate the metadata so the "(dirty)" suffix is removed, or
simply remove the duplicate/incorrect "(dirty)" marker from the metadata file so
the Git line accurately reflects a clean state.
In `@tests/CE7B0BC7/golden-metadata.txt`:
- Line 29: The Fypp entry in golden-metadata.txt contains a personal HPC scratch
absolute path; replace that absolute path with a non-personal, reproducible
reference (e.g., just "fypp" or a configurable/path variable) so the metadata
does not expose a user-specific location—update the "Fypp" field value
accordingly and ensure any CI/tests use the generic/tool name or
environment-driven path instead of the original /storage/... user path.
- Line 163: The line containing the dynamic field "CPU(s) scaling MHz" in the
golden metadata is unstable across runs; update the test golden generation or
assertion to normalize or ignore this field by replacing the numeric percentage
with a stable placeholder (e.g., a wildcard or fixed token) or removing the
"CPU(s) scaling MHz" line from the comparison, and update the code that
builds/validates golden outputs so "CPU(s) scaling MHz" is not treated as a
strict exact-match value.
- Line 7: The golden metadata contains a transient "dirty" working-tree marker
in the Git line ("c9cff0655061b66969305f06a0db63138eeda7e4 on
fix/time-stepping-order (dirty)") which makes the test flaky; update the test
fixture to remove the " (dirty)" suffix (or otherwise normalize/strip
working-tree state) so the expected string matches a clean commit hash and
branch name—look for the golden-metadata entry that contains
"c9cff0655061b66969305f06a0db63138eeda7e4 on fix/time-stepping-order (dirty)"
and change it to "c9cff0655061b66969305f06a0db63138eeda7e4 on
fix/time-stepping-order" (or implement normalization code where the metadata is
produced).
In `@tests/D80F2162/golden-metadata.txt`:
- Line 7: The golden-metadata.txt was created from a dirty working tree (commit
c9cff0655061b66969305f06a0db63138eeda7e4 on fix/time-stepping-order), so
regenerate the golden files from a clean state: checkout or stash/commit your
local changes, ensure the workspace is clean (git status shows no
modifications), re-run the golden-file generation step that produces
tests/D80F2162/golden-metadata.txt, and verify the regenerated file matches
expected outputs; also check the related tests/53A15FFC/golden-metadata.txt for
the same issue to avoid repeating dirty-state artifacts.
In `@tests/DFF6E349/golden-metadata.txt`:
- Line 29: The golden-metadata.txt contains a personal HPC scratch path in the
"Fypp" field; replace the hard-coded absolute path shown in the Fypp entry with
a generic or project-relative path (e.g., just the executable name "fypp", a
path under the repo, or an environment-variable-based placeholder) so the
metadata does not contain user-specific locations; update the "Fypp" line in
golden-metadata.txt accordingly.
- Line 163: The golden metadata contains a dynamic field "CPU(s) scaling MHz"
(value 91% here) that will vary between runs; update the test golden to
normalize this by replacing the literal value with a stable placeholder or
pattern (e.g., "<DYNAMIC_CPU_SCALING>" or a regex) and update the test assertion
that reads "CPU(s) scaling MHz" to accept that placeholder/pattern instead of a
fixed percentage so test results are deterministic.
- Line 7: The golden metadata contains a dirty working-tree marker ("Git:
c9cff0655061b66969305f06a0db63138eeda7e4 on fix/time-stepping-order (dirty)");
remove the "(dirty)" marker from tests/DFF6E349/golden-metadata.txt (or
regenerate the file from a clean commit) and/or change the metadata generation
logic so it uses a stable git identifier (e.g., git rev-parse --short HEAD or
git describe without the dirty flag) to ensure the "Git: ..." line does not
include the "(dirty)" suffix.
---
Nitpick comments:
In `@tests/B9553426/golden-metadata.txt`:
- Around line 176-192: The CI host lscpu output shows multiple unmitigated CPU
vulnerabilities (notably Spectre v2 with IBPB/STIBP disabled, Retbleed, Gather
data sampling, Mmio stale data, Spec store bypass, Indirect target selection,
Vmscape); either harden the CI node by asking infra to enable kernel mitigations
via kernel boot parameters (e.g., spectre_v2=on ibpb=on stibp=on
spec_store_bypass_disable=on) or explicitly document the accepted risk for the
CI environment in the project/infrastructure docs and open an infrastructure
ticket referencing this golden-metadata output (call out “Spectre v2:
IBPB/STIBP”, “Retbleed”, “Gather data sampling”, “Mmio stale data”, “Spec store
bypass”, “Indirect target selection”, “Vmscape”) so the cluster admins can
evaluate and, if chosen, apply the suggested boot params.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1232 +/- ##
==========================================
+ Coverage 45.34% 45.36% +0.01%
==========================================
Files 70 70
Lines 20514 20514
Branches 1954 1953 -1
==========================================
+ Hits 9303 9306 +3
+ Misses 10084 10083 -1
+ Partials 1127 1125 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
codeant-ai
bot
added
size:XXL
sbryngelson
force-pushed
the
fix/time-stepping-order
branch
from
930e2c5 to
2dd4392
Compare
codeant-ai
bot
added
size:XXL
sbryngelson
force-pushed
the
fix/time-stepping-order
branch
2 times, most recently
from
0107c0e to
417d980
Compare
codeant-ai
bot
added
size:XXL
sbryngelson
force-pushed
the
fix/time-stepping-order
branch
from
f717118 to
c10cca7
Compare
Move mytime update to after the TVD RK call so that sub-step source terms see the correct time. Also change acoustic source to use mytime instead of t_step*dt, which was inconsistent with adaptive time stepping. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Update golden files for the 1D/2D/3D Bodyforces cfl_adap_dt=T tests to reflect the corrected mytime ordering (incremented after RK integration rather than before). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The mytime update now happens after RK stepping instead of before, which fixes the last-timestep RHS being skipped for cfl_dt tests. This affects QBMM, Lagrange bubbles, body forces, cylindrical, and example tests that use cfl_dt/cfl_adap_dt. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
No longer needed after switching from sim_time = t_step*dt to sim_time = mytime. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Golden files were previously generated across different machines (macOS/gfortran 15 vs Linux/gfortran 12, MPI ON vs OFF). Regenerate all 25 affected metadata files on a single canonical environment (Phoenix, gfortran 12.3.0, openmpi 4.1.5, MPI=Yes) for consistency. Numerical golden.txt values are unchanged — only metadata updated. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Remove copy-paste WENO/QP docstrings from acoustic source params - Add "why" comments on mytime placement and sim_time = mytime - Fix typos: "hane" -> "hand", "Temrs" -> "Terms", "pphysics" -> "physics", "sequel" -> "sequential" - Fix missing @ on Doxygen @param in m_body_forces.fpp Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
sbryngelson
force-pushed
the
fix/time-stepping-order
branch
from
c595911 to
aba125c
Compare
Claude Code ReviewHead SHA: ed98cb0 Files changed: 54 total — 4 source files + 50 golden test files Key source files:
Summary
Findings1. Multi-stage RK sub-step time accuracy — design note ( With the fix, 2. Golden files regenerated on a different platform ( The old golden files were generated on macOS with Apple M4 Pro / gfortran 15.2, and the new ones on GT Phoenix (x86_64 / gfortran 12.3). The metadata diffs confirm this. Platform-switching changes can mask or conflate floating-point differences unrelated to this fix. The broad scope of numerical changes (e.g., tests 3. Some CI checks are failing The PR currently shows failures in 4. These arguments appear to be read-only in the subroutine (no write to them was visible in the prior code). If the subroutine does not modify them, VerdictThe core logic is correct: moving |
Claude Code ReviewHead SHA: ed98cb0 Files changed: 54 (4 source files + 50 test golden files) Source files:
Summary
FindingsNo blocking issues found. The changes are correct and well-scoped. Minor observations
Verdict: Correct bug fix. Fixes a one-step time lag in source-term evaluation and a wrong time formula under adaptive |
📝 WalkthroughWalkthroughThis pull request modifies the acoustic source calculation interface and time-stepping order in the simulation code. The 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can approve the review once all CodeRabbit's comments are resolved.Enable the |
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (2)
tests/70EC99CE/golden-metadata.txt (1)
7-7:⚠️ Potential issue | 🟡 MinorRegenerate golden metadata from a clean tree for reproducibility.
(dirty)in the recorded Git state means this golden snapshot cannot be reproduced from the commit hash alone. Please regenerate after committing/stashing local changes so metadata points to a clean revision.tests/B54BB9D8/golden-metadata.txt (1)
7-7:⚠️ Potential issue | 🟡 MinorRegenerate this metadata from a clean checkout.
Line 7 still records
(dirty), so this baseline is not reproducible and may include uncommitted local changes. Please rerun the golden generation from a clean tree, and apply the same cleanup to the other regeneratedgolden-metadata.txtfiles in this PR.
🧹 Nitpick comments (3)
tests/0D1FA5C5/golden-metadata.txt (1)
29-29: Avoid committing user-specific absolute tool paths in golden metadata.Lines 29, 63, and 97 embed a home-directory path (
/storage/home/.../sbryngelson3/.../fypp), which can leak user identifiers and create unnecessary golden churn across environments. Consider normalizing/redacting this field in generated metadata.Also applies to: 63-63, 97-97
tests/DFF6E349/golden-metadata.txt (1)
13-16: Consider normalizing host/path/toolchain fields in golden metadataThese lines encode machine-local hostnames and absolute filesystem paths, which creates high-churn golden diffs and exposes internal infrastructure details without improving numerical validation. Recommend replacing volatile values with stable placeholders (or excluding them from golden comparison).
Possible normalization approach
- CMake v3.26.5 on atl1-1-02-007-5-2.pace.gatech.edu + CMake v3.26.5 on <HOST> - Fypp : /storage/home/hcoda1/6/sbryngelson3/scratch/MFC-prs/build/venv/bin/fypp + Fypp : <FYPP_PATH> - CC : /usr/local/pace-apps/spack/packages/.../bin/gcc + CC : <CC_PATH>Also applies to: 29-29, 36-38, 47-50, 63-63, 70-72, 81-84, 97-97, 104-106
tests/128954AD/golden-metadata.txt (1)
13-16: Consider normalizing host/user/toolchain absolute paths to reduce golden churn.These machine-specific fields will keep changing across environments and can create noisy golden updates unrelated to physics/time-stepping behavior. If possible, scrub or template volatile values in golden metadata comparison/output.
Also applies to: 29-29, 36-38, 47-50, 63-63, 70-72, 81-84, 97-97, 104-106
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4d9fc1b4-0a95-4fc3-9393-8c8eddf32b27
📒 Files selected for processing (54)
src/simulation/m_acoustic_src.fppsrc/simulation/m_body_forces.fppsrc/simulation/m_rhs.fppsrc/simulation/m_start_up.fpptests/016C1B8B/golden-metadata.txttests/016C1B8B/golden.txttests/0501B3DA/golden-metadata.txttests/0501B3DA/golden.txttests/07C54EDD/golden-metadata.txttests/07C54EDD/golden.txttests/0D1FA5C5/golden-metadata.txttests/0D1FA5C5/golden.txttests/10041BB7/golden-metadata.txttests/10041BB7/golden.txttests/121D4ECA/golden-metadata.txttests/121D4ECA/golden.txttests/128954AD/golden-metadata.txttests/128954AD/golden.txttests/12ECE133/golden-metadata.txttests/12ECE133/golden.txttests/2122A4F6/golden-metadata.txttests/2122A4F6/golden.txttests/37FA2CEF/golden-metadata.txttests/37FA2CEF/golden.txttests/4A1BD9B8/golden-metadata.txttests/4A1BD9B8/golden.txttests/53A15FFC/golden-metadata.txttests/53A15FFC/golden.txttests/6B1AD553/golden-metadata.txttests/6B1AD553/golden.txttests/70EC99CE/golden-metadata.txttests/70EC99CE/golden.txttests/986BC1A2/golden-metadata.txttests/986BC1A2/golden.txttests/A078904B/golden-metadata.txttests/A078904B/golden.txttests/AE02324F/golden-metadata.txttests/AE02324F/golden.txttests/B2EC143C/golden-metadata.txttests/B2EC143C/golden.txttests/B54BB9D8/golden-metadata.txttests/B54BB9D8/golden.txttests/B9553426/golden-metadata.txttests/B9553426/golden.txttests/C7927D03/golden-metadata.txttests/C7927D03/golden.txttests/CE7B0BC7/golden-metadata.txttests/CE7B0BC7/golden.txttests/D80F2162/golden-metadata.txttests/D80F2162/golden.txttests/DFF6E349/golden-metadata.txttests/DFF6E349/golden.txttests/E21940B3/golden-metadata.txttests/E21940B3/golden.txt
| ! Advance time after RK so source terms see current-step time | ||
| mytime = mytime + dt | ||
|
|
There was a problem hiding this comment.
Pass a stage time into RHS/source evaluation instead of reusing global mytime.
Line 854 fixes the one-step offset, but mytime is still updated only once after the full RK step. src/simulation/m_time_steppers.fpp calls s_compute_rhs once per RK stage, while src/simulation/m_acoustic_src.fpp and src/simulation/m_body_forces.fpp both derive forcing time from mytime only. For SSP/TVD RK2/3, that keeps time-dependent forcing at t_n on every stage instead of t_n + c_s*dt, so acoustic/body-force integration remains first-order in time. Please thread an explicit stage time through RHS/source evaluation. As per coding guidelines, "PR-level guidelines emphasize timing correctness in RK sub-steps and the need to regenerate golden outputs for acoustic sources/body forces when timing semantics change."
| OMPI_FC : | ||
| Invocation: test --generate --only 128954AD -j 8 --no-build -- -c phoenix | ||
| Lock: mpi=Yes & gpu=No & debug=No & gcov=No & unified=No & single=No & mixed=No & fastmath=No | ||
| Git: 0ce91cc551508e01d2fc4127caeb9ac010cfa258 on fix/time-stepping-order (dirty) |
There was a problem hiding this comment.
Regenerate golden metadata from a clean tree (remove dirty provenance).
This snapshot was captured from a dirty working directory, so it cannot be traced to a single reproducible source state. Please regenerate after committing/stashing local changes.
| Git: 17eb3def840695d311145e1bbeb6d5b38cb74674 on dt_bug_fixes (dirty) | ||
| Invocation: test --generate --only 53A15FFC -j 8 --no-build -- -c phoenix | ||
| Lock: mpi=Yes & gpu=No & debug=No & gcov=No & unified=No & single=No & mixed=No & fastmath=No | ||
| Git: 0ce91cc551508e01d2fc4127caeb9ac010cfa258 on fix/time-stepping-order (dirty) |
There was a problem hiding this comment.
Regenerate golden metadata from a clean working tree.
The baseline metadata is marked (dirty), which makes this golden artifact non-reproducible and can hide unintended local edits in the reference output. Please regenerate after ensuring git status is clean.
Suggested update
- Git: 0ce91cc551508e01d2fc4127caeb9ac010cfa258 on fix/time-stepping-order (dirty)
+ Git: 0ce91cc551508e01d2fc4127caeb9ac010cfa258 on fix/time-stepping-order📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Git: 0ce91cc551508e01d2fc4127caeb9ac010cfa258 on fix/time-stepping-order (dirty) | |
| Git: 0ce91cc551508e01d2fc4127caeb9ac010cfa258 on fix/time-stepping-order |
| Git: 384d0b161360ac4b41f52be317846746fd1b5d33 on threeNewExamples (clean) | ||
| Invocation: test --generate --only 986BC1A2 -j 8 --no-build -- -c phoenix | ||
| Lock: mpi=Yes & gpu=No & debug=No & gcov=No & unified=No & single=No & mixed=No & fastmath=No | ||
| Git: 0ce91cc551508e01d2fc4127caeb9ac010cfa258 on fix/time-stepping-order (dirty) |
There was a problem hiding this comment.
Golden file generated from dirty working tree.
The "(dirty)" flag indicates uncommitted changes were present during baseline generation, which breaks reproducibility. Golden files should be regenerated from a clean commit to ensure the baseline can be recreated by future developers and CI systems.
| CMake v3.26.5 on atl1-1-02-007-5-2.pace.gatech.edu | ||
|
|
||
| C : NVHPC v24.5.0 (/usr/local/pace-apps/manual/packages/nvhpc/24.5/Linux_x86_64/24.5/compilers/bin/nvc) | ||
| Fortran : NVHPC v24.5.0 (/usr/local/pace-apps/manual/packages/nvhpc/24.5/Linux_x86_64/24.5/compilers/bin/nvfortran) | ||
| C : GNU v12.3.0 (/usr/local/pace-apps/spack/packages/linux-rhel9-x86_64_v3/gcc-11.3.1/gcc-12.3.0-ukkkutsxfl5kpnnaxflpkq2jtliwthfz/bin/gcc) | ||
| Fortran : GNU v12.3.0 (/usr/local/pace-apps/spack/packages/linux-rhel9-x86_64_v3/gcc-11.3.1/gcc-12.3.0-ukkkutsxfl5kpnnaxflpkq2jtliwthfz/bin/gfortran) | ||
|
|
||
| PRE_PROCESS : ON | ||
| SIMULATION : OFF | ||
| PRE_PROCESS : OFF | ||
| SIMULATION : ON | ||
| POST_PROCESS : OFF | ||
| SYSCHECK : OFF | ||
| DOCUMENTATION : OFF | ||
| ALL : OFF | ||
|
|
||
| MPI : ON | ||
| OpenACC : ON | ||
| OpenACC : OFF | ||
| OpenMP : OFF | ||
|
|
||
| Fypp : /storage/home/hcoda1/7/mzhang775/r-sbryngelson3-0/MFCMarkZhang/build/venv/bin/fypp | ||
| Fypp : /storage/home/hcoda1/6/sbryngelson3/scratch/MFC-prs/build/venv/bin/fypp | ||
| Doxygen : | ||
|
|
||
| Build Type : Release | ||
|
|
||
| Configuration Environment: | ||
|
|
||
| CC : /usr/local/pace-apps/manual/packages/nvhpc/24.5/Linux_x86_64/24.5/compilers/bin/nvc | ||
| CXX : /usr/local/pace-apps/manual/packages/nvhpc/24.5/Linux_x86_64/24.5/compilers/bin/nvc++ | ||
| FC : /usr/local/pace-apps/manual/packages/nvhpc/24.5/Linux_x86_64/24.5/compilers/bin/nvfortran | ||
| CC : /usr/local/pace-apps/spack/packages/linux-rhel9-x86_64_v3/gcc-11.3.1/gcc-12.3.0-ukkkutsxfl5kpnnaxflpkq2jtliwthfz/bin/gcc | ||
| CXX : /usr/local/pace-apps/spack/packages/linux-rhel9-x86_64_v3/gcc-11.3.1/gcc-12.3.0-ukkkutsxfl5kpnnaxflpkq2jtliwthfz/bin/g++ | ||
| FC : /usr/local/pace-apps/spack/packages/linux-rhel9-x86_64_v3/gcc-11.3.1/gcc-12.3.0-ukkkutsxfl5kpnnaxflpkq2jtliwthfz/bin/gfortran |
There was a problem hiding this comment.
Environment migration mixed with timing fix validation.
The metadata shows a complete toolchain and hardware migration (different host node, NVHPC→GNU compiler switch, different build paths) occurring simultaneously with the timing fix. This conflates two unrelated changes:
- Timing fix: The PR's functional change requiring golden file updates
- Infrastructure migration: Hardware/compiler environment change
Different compilers and hardware produce different floating-point results even for identical source code, making it impossible to verify that output differences stem solely from the timing fix rather than the environment change.
Recommendation: Regenerate golden files either in the original environment or in a separate PR dedicated to infrastructure migration. If the environment change is intentional and permanent, document the rationale and verify the timing fix separately.
Also applies to: 47-72, 81-106
| OMPI_FC : | ||
| Invocation: test --generate --only B2EC143C -j 8 --no-build -- -c phoenix | ||
| Lock: mpi=Yes & gpu=No & debug=No & gcov=No & unified=No & single=No & mixed=No & fastmath=No | ||
| Git: 0ce91cc551508e01d2fc4127caeb9ac010cfa258 on fix/time-stepping-order (dirty) |
There was a problem hiding this comment.
Avoid committing golden metadata from a dirty tree
(dirty) makes this artifact non-reproducible and guarantees churn between runs. Please regenerate from a clean checkout before updating golden metadata.
| Invocation: test --generate --only B9553426 -j 8 --no-build -- -c phoenix | ||
| Lock: mpi=Yes & gpu=No & debug=No & gcov=No & unified=No & single=No & mixed=No & fastmath=No | ||
| Git: 290200003ad4b30aa99f3c261cbf20a908ce3c6e on lag-swap (dirty) | ||
| Git: 0ce91cc551508e01d2fc4127caeb9ac010cfa258 on fix/time-stepping-order (dirty) |
There was a problem hiding this comment.
Regenerate golden metadata from a clean tree
Line 7 shows Git: ... (dirty). Baselines generated from uncommitted changes are not reliably reproducible and can mask what commit actually produced the golden output. Please regenerate this metadata with a clean working tree.
| OpenMP : OFF | ||
|
|
||
| Fypp : /Users/hyeoksu/MyWork/MFC-local/MFC/lag-swap/build/venv/bin/fypp | ||
| Fypp : /storage/home/hcoda1/6/sbryngelson3/scratch/MFC-prs/build/venv/bin/fypp |
There was a problem hiding this comment.
Avoid committing user-specific absolute paths in golden metadata
Lines 29, 63, and 97 embed a personal absolute path (/storage/home/.../sbryngelson3/...). This is environment-specific and leaks user/infrastructure details into versioned artifacts. Prefer redacted/normalized tool paths (or a stable placeholder) in committed golden metadata.
Also applies to: 63-63, 97-97
| Git: 23ee7b6c702ed08bc2b7281173943308d43772af on CFLDT (dirty) | ||
| Invocation: test --generate --only D80F2162 -j 8 --no-build -- -c phoenix | ||
| Lock: mpi=Yes & gpu=No & debug=No & gcov=No & unified=No & single=No & mixed=No & fastmath=No | ||
| Git: 0ce91cc551508e01d2fc4127caeb9ac010cfa258 on fix/time-stepping-order (dirty) |
There was a problem hiding this comment.
Regenerate golden metadata from a clean worktree
The recorded git state is (dirty), which makes this baseline harder to reproduce and audit later. Please regenerate this metadata with a clean checkout/branch state before merge.
| OMPI_CC : | ||
| OMPI_CXX : | ||
| OMPI_FC : | ||
| Git: 0ce91cc551508e01d2fc4127caeb9ac010cfa258 on fix/time-stepping-order (dirty) |
There was a problem hiding this comment.
Avoid committing golden metadata generated from a dirty workspace
Line 7 shows (dirty), which means this baseline was captured with uncommitted local changes. That makes provenance and reproducibility ambiguous for future test updates. Please regenerate this golden metadata from a clean tree.
Claude Code ReviewHead SHA: d9bab25 Source files:
Summary
Findings1. Correctness of the timing fix —
|
Claude Code ReviewHead SHA: Files changed: 54 (4 source files + 50 golden file updates) Source files:
Summary
Findings1. Correctness (medium): RK sub-step timing remains approximate — PR claim overstates the fix
All RK stages now uniformly evaluate sources at 2. Correctness (low):
Using the module-global 3. Observation: Golden files regenerated on a different platform/compiler version All 50 golden files were regenerated on Phoenix (gfortran 12.3.0, Linux x86_64), replacing goldens previously generated on macOS (gfortran 15.2.0, Apple M4 Pro). The diffs conflate the timing fix with platform-specific floating-point differences. If CI runs on the same Phoenix platform this is fine; tolerances should absorb any remaining differences. 4. Minor:
OverallThe core fix ( |
User description
Summary
mytime = mytime + dtfrom before the TVD RK call to after it, so that RK sub-steps see the correct time for source terms and body forcest_step*dttomytime, which is correct under adaptive time steppingTest plan
🤖 Generated with Claude Code
CodeAnt-AI Description
Fix timing of acoustic sources and body-force evaluation during Runge–Kutta sub-steps
What Changed
Impact
✅ Accurate time-dependent acoustic forcing with adaptive timesteps✅ Correct body-force evaluation timing during Runge–Kutta sub-steps✅ Updated golden outputs for tests that use adaptive timestepping💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.